feat: improve external post embeds#2794
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds HLS-based video playback: a new VideoPlayer component using hls.js with viewport-based autoplay, integrates it into BlueskyPostEmbed to render playlist-backed videos, and updates CSP media and connect sources, dependencies, and tests accordingly. ChangesVideo Streaming for Bluesky Embeds
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📊 Dependency Size ChangesWarning This PR adds 24.8 MB of new dependencies, which exceeds the threshold of 200 kB.
Total size change: 24.8 MB |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/global/BlueskyPostEmbed.client.vue (1)
200-213: 💤 Low valueConsider whether viewport-based autoplay is desired.
The
VideoPlayercomponent supports anautoplayprop that triggers play/pause based on viewport intersection, but it isn't passed here. If auto-play when scrolling into view is desired, add:autoplay="true". The current setup requires manual user interaction, which may be intentional given thecontrolsandmutedattributes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/global/BlueskyPostEmbed.client.vue` around lines 200 - 213, The template rendering a VideoPlayer for playlist embeds omits the component's viewport-based autoplay prop, so the video won't auto-play when scrolled into view; decide whether autoplay-on-intersection is desired and if so add the :autoplay="true" prop to the <VideoPlayer> usage (keeping existing props like controls, muted, loop) to enable the component's intersection-driven play/pause behavior, otherwise leave as-is to require manual interaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/VideoPlayer.vue`:
- Around line 22-24: The HLS error handler currently logs only the event name;
update the HLS listener registered with hls.on(Hls.Events.ERROR, ...) to accept
both parameters (event, data) and log the actual error details from data (e.g.,
data.type, data.details, data.response) instead of the event string, and handle
fatal errors by calling the Hls instance recovery/cleanup methods (e.g.,
hls.recoverMediaError(), hls.startLoad() or hls.destroy()) depending on
data.fatal; modify the callback signature and add branching for data.fatal to
attempt recovery or cleanup accordingly.
---
Nitpick comments:
In `@app/components/global/BlueskyPostEmbed.client.vue`:
- Around line 200-213: The template rendering a VideoPlayer for playlist embeds
omits the component's viewport-based autoplay prop, so the video won't auto-play
when scrolled into view; decide whether autoplay-on-intersection is desired and
if so add the :autoplay="true" prop to the <VideoPlayer> usage (keeping existing
props like controls, muted, loop) to enable the component's intersection-driven
play/pause behavior, otherwise leave as-is to require manual interaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa5c26f3-6d23-47e9-9bee-fbd45cb38d85
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
app/components/VideoPlayer.vueapp/components/global/BlueskyPostEmbed.client.vuemodules/security-headers.tspackage.json
ghostdevv
left a comment
ghostdevv
left a comment
There was a problem hiding this comment.
I assume it's not possible to just use the video element with a link to the blob directly, rather than using hls.js? If not then I think we shouldn't load the library or the video until the user clicks on it
|
@ghostdevv this is the same approach as in bluesky - loads the logic immediately, and the video when it appears on the screen (with lazy attr) Otherwise, you'd have to load the library first, and then the video would start loading. I'm not entirely sure that the optimization for those who didn't click justifies the experience for those who did... |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
2 participants
🧭 Context
Since I always think about improving the blog at the last minute, I'm doing it separately this time 🙃
Added full video support and improved post zones clickability. Now, in addition to video, we can also click to external embeds.
For video, I used an external library. It's the base approach for atproto projects, and I use it in mine as well